Skip to content

jit: #1093 review follow-ups on the operation dump, the slice-index match, and the closure-select fixtures - #1110

Merged
youknowone merged 1 commit into
mainfrom
residual
Aug 8, 2026
Merged

jit: #1093 review follow-ups on the operation dump, the slice-index match, and the closure-select fixtures#1110
youknowone merged 1 commit into
mainfrom
residual

Conversation

@youknowone

Copy link
Copy Markdown
Owner

Follow-up to #1093, which merged before its review tail was closed. One commit, majit-translate only.

Seven findings were open on #1093. Five are fixed here, one was already moot, one is declined with evidence.

Fixed

tool/error.rs — the operation dump could omit the failing operation.
no_source_lines listed the first 40 operations of a block. A block whose failure sits past operation 40 therefore showed neither the failing op nor the ops that produced its operands, which is the entire point of the dump. It now takes the failing operation's index and ends the window there, reporting the elided prefix as … N earlier operation(s). All five call sites pass the index.

A 60-operation regression test with operindex = 55 asserts op[55] and op[54] are listed, op[56] is not, the listing stays exactly 40 long, and both elision lines read … 16 earlier operation(s) / … 4 more operation(s). The fixture sets graph.func = None so every source accessor errors and the dump path is the one under test.

front/slice_index.rs — nested if let + inner match.
Collapsed into a single match, which removes the SliceIndexBounds::MinusOne => unreachable!() arm. The two unreachable!()s at :214 and :673 are unrelated (they re-destructure an already-matched OpKind::Call) and are left alone.

front/mir.rs — the anchor's name contradicted its body.
call_function_impl_result_has_no_residual_array_index asserted that the residual index call remains. Renamed to call_function_impl_result_keeps_residual_array_index.

The review also asked for an index_mut assertion. I probed the real graph rather than adding it blind: call_function_impl_result contains no index_mut at all. Its array/slice calls are core::array::<Impl>::index ×1, __array_repeat ×1, core::slice::iter ×4 and Vec::extend_from_slice ×2. The request and the pre-existing doc comment were both unfounded, so the comment is corrected and the verifiable paired negative __getslice_rangeto == 0 is asserted instead — the fold is declined, so no site may be rewritten.

front/option_closure_select.rs — the fixtures could not distinguish the two owners.
Receiver and result shared one owner root, so an assertion about "the result enum" would have passed against the receiver's. The fixtures now use test::recv::Option and test::result::Option, and the tests assert FieldWrite::owner_root for __discriminant and __pos_0.

Mutation-proven rather than asserted-and-hoped: swapping result_option_owner back to option_owner fails the test with left: ["test::recv::Option", "test::result::Option"] vs right: ["test::result::Option"].

The __pos_0 owner list has two entries — the second __pos_0 write is the closure's (x,) Args tuple from emit_call_once, a real Tuple unrelated to either Option instantiation. That is asserted explicitly so the extra entry cannot be mistaken for leakage.

front/option_closure_select.rsresult_niche: true was never exercised.
Two tests added for map and and_then, asserting no synthetic constructor, no __discriminant write, and exactly one core::ptr::null_mut call. They cannot pass on the non-niche path.

Moot

slice_index.rs:920's == false predicate was already removed by e1fc067eae2.

Declined

flowspace_adapter.rs:4740 — the suggestion was to change the fixture's ValueType::Int. front::mir.rs's Rvalue::Repeat really does emit result_ty: ValueType::Int for __array_repeat, and git show 128590c675f confirms that declaration predates this work, so the fixture mirrors what it models. Changing it would make the fixture disagree with the lowering. The graph-level contract is anchored by the sibling real-LLBC test directly below it.

Verification

  • cargo test --release -p majit-translate --lib — 3132 passed, 0 failed
  • cargo test --all --no-default-features --features dynasm — 102 suites, 7521 passed, 0 failed (run at base fd658cb0723; the base has since advanced by jit(wasm): drop the wasm32 arm of the self-recursive root-bridge inline #1106, which this commit does not touch)
  • The renamed real-corpus anchor re-run against a freshly extracted corpus
  • cargo fmt --check clean

authored by Claude

…atch, and the closure-select fixtures

no_source_lines takes the failing operation's index and ends its 40-op
window there, so a block whose failure sits past the window still shows
that op and the ops that produced its operands; the elided prefix is
reported as "… N earlier operation(s)".  All five call sites pass the
index.  A 60-operation regression test with operindex 55 asserts op[55]
and op[54] are listed, op[56] is not, and the listing stays 40 long.

rewire_one_slice_index_rangeto_site's nested `if let` plus inner `match`
becomes a single `match`, which removes the MinusOne unreachable!().

call_function_impl_result_has_no_residual_array_index asserted that the
residual index call remains, so it is renamed
call_function_impl_result_keeps_residual_array_index and its doc comment
is corrected.  The comment claimed an index_mut call that this graph does
not contain; the graph's array/slice calls are core::array::<Impl>::index
x1, __array_repeat x1, core::slice::iter x4 and extend_from_slice x2.
The paired negative __getslice_rangeto == 0 is asserted instead.

option_closure_select's fixtures give the result enum a different owner
root (test::result::Option) from the receiver's (test::recv::Option), and
the tests assert FieldWrite::owner_root for __discriminant and __pos_0.
Two tests cover result_niche: true, asserting no synthetic constructor,
no __discriminant write and one core::ptr::null_mut call.

Assisted-by: Claude
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 51b159de-8426-4261-8373-34ef220da82e

📥 Commits

Reviewing files that changed from the base of the PR and between 4555e3d and 633e6b1.

📒 Files selected for processing (4)
  • majit/majit-translate/src/front/mir.rs
  • majit/majit-translate/src/front/option_closure_select.rs
  • majit/majit-translate/src/front/slice_index.rs
  • majit/majit-translate/src/tool/error.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 633e6b1).
Updated: 2026-08-08T01:35:23.415Z

Files in the reviewed diff
majit/majit-translate/src/front/mir.rs
majit/majit-translate/src/front/option_closure_select.rs
majit/majit-translate/src/front/slice_index.rs
majit/majit-translate/src/tool/error.rs

Codex did not produce a report (exit 1). Last log lines:

reasoning summaries: none
session id: 019fdf02-a78f-74b3-a5a0-a3aa838ea09b
--------
user
Assess by static analysis whether our changes in git diff upstream/main are equivalent
to the corresponding RPython/PyPy source code. The RPython and PyPy sources
are available locally (under `rpython/` and `pypy/` in this repository).

If anything was ported incorrectly, report every instance in detail. After
collecting all differences, organize the report into separate sections:

1. Cases where our patch regressed PyPy parity compared to main
2. Other mismatches introduced by our patch
3. Mismatches that already existed before this patch
4. Structural adaptations

Exceptions: some differences cannot be ported 1:1 because of Python 3.11 vs
3.14 differences, opcode mismatches caused by using a CPython-compatible
compiler, GIL/free-threading differences, and fundamental implementation-
language differences between RPython and Rust. Mark those separately under
"Structural adaptations".

Scope discipline: before writing the report, run
`git diff upstream/main --name-only -- . ':(exclude)*.jitstats'` and treat that
file list as the authoritative definition of "this patch" (when an authoritative
changed-file list is appended below, use that instead of re-deriving it). The
excluded `*.jitstats` files are `pyre/check.py`'s recorded jit-stats baselines —
generated golden data with no RPython/PyPy counterpart, so no parity finding can
cite one, and a bulk re-record of them is not a change to review. Findings under
sections 1 and 2 MUST cite our-side files from that list; a divergence in any
file NOT in the list is by definition not introduced by this patch — report
it under section 3 instead, or omit it. Verify every section-1/2 citation
against the list before finalizing the report.

---

Output format requirements (so the report can be parsed mechanically and
posted/triaged automatically). Use these four headings VERBATIM, in this
order, and nothing else at heading level 2:

## 1. Regressions to PyPy parity introduced by this patch
## 2. Other mismatches introduced by this patch
## 3. Pre-existing mismatches (already present before this patch)
## 4. Structural adaptations

Under each heading, list every finding as a bullet. For each finding cite the
concrete `our_file.rs:line ↔ rpython_or_pypy_file.py:line` pair and quote the
divergence concisely. If a section has no findings, still emit the heading
followed by a single line `None.` so all four sections are always present.
Do not modify any files; produce the report only.

Authoritative changed-file list for this patch (git diff upstream/main --name-only,
minus 0 generated `*.jitstats` baseline file(s)):
majit/majit-translate/src/front/mir.rs
majit/majit-translate/src/front/option_closure_select.rs
majit/majit-translate/src/front/slice_index.rs
majit/majit-translate/src/tool/error.rs
warning: Codex could not find bubblewrap on PATH. Install bubblewrap with your OS package manager. See the sandbox prerequisites: https://developers.openai.com/codex/concepts/sandboxing#prerequisites. Codex will use the bundled bubblewrap in the meantime.
ERROR: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 3:35 AM.
ERROR: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 3:35 AM.

@youknowone
youknowone merged commit 436631e into main Aug 8, 2026
14 of 17 checks passed
@youknowone
youknowone deleted the residual branch August 8, 2026 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant